Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add timer state code+ut #399

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add timer state code+ut #399

wants to merge 4 commits into from

Conversation

M-iceberg
Copy link
Contributor

No description provided.

@M-iceberg M-iceberg changed the title add code add timer state code+ut Oct 25, 2024
@M-iceberg
Copy link
Contributor Author

M-iceberg commented Oct 25, 2024 •
edited
Loading

/azp run #Resolved

@azure-pipelines Azure Pipelines
Copy link

azure-pipelines bot commented Oct 25, 2024 •
edited by M-iceberg
Loading

Azure Pipelines successfully started running 1 pipeline(s).

#Resolved

@M-iceberg
Copy link
Contributor Author

M-iceberg commented Oct 25, 2024 •
edited
Loading

/azp run #Resolved

@azure-pipelines Azure Pipelines
Copy link

azure-pipelines bot commented Oct 25, 2024 •
edited by M-iceberg
Loading

Azure Pipelines successfully started running 1 pipeline(s).

#Resolved

jasmineymlo
jasmineymlo reviewed Oct 25, 2024
View reviewed changes
win32/src/threadpool_win32.c
)
{
LogError("timer is in closing state");
result = MU_FAILURE;
Copy link
Contributor

@jasmineymlo jasmineymlo Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we shouldn't return MU_FAILURE if it's due to closing...otherwise will fault...this is breaking change and behaviour is different than what we have previously.
need to confirm with Dan

@jasmineymlo
Copy link
Contributor 

    threadpool_internal_set_timer(timer->timer, start_delay_ms, timer_period_ms);

seems like we could still have destroy going after we check...then we would end up in same situation?

Refers to: win32/src/threadpool_win32.c:641 in ef31a77. [](commit_id = ef31a77, deletion_comment = False)

dcristoloveanu
dcristoloveanu reviewed Oct 25, 2024
View reviewed changes
win32/devdoc/threadpool_win32.md
@@ -215,7 +215,7 @@ MOCKABLE_FUNCTION(, int, threadpool_timer_restart, TIMER_INSTANCE_HANDLE, timer,

**SRS_THREADPOOL_WIN32_42_019: [** If `timer` is `NULL`, `threadpool_timer_restart` shall fail and return a non-zero value. **]**

If timer state is `DESTROYING`, `threadpool_timer_restart` shall fail and return a non-zero value.
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DESTROYING

These states should be described at the top of the .md (Otherwise when reading this, I have no idea when we switch to the state and when we transition out of it) ... #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

dcristoloveanu
dcristoloveanu reviewed Oct 25, 2024
View reviewed changes
win32/devdoc/threadpool_win32.md
@@ -215,7 +215,7 @@ MOCKABLE_FUNCTION(, int, threadpool_timer_restart, TIMER_INSTANCE_HANDLE, timer,

**SRS_THREADPOOL_WIN32_42_019: [** If `timer` is `NULL`, `threadpool_timer_restart` shall fail and return a non-zero value. **]**

If timer state is `DESTROYING`, `threadpool_timer_restart` shall fail and return a non-zero value.
**SRS_THREADPOOL_WIN32_07_001: [** If timer state is in destroying state, `threadpool_timer_restart` shall fail and return a non-zero value. **]**
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroying

I think capital and enclosed by tickmarks was better. #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are references to states that should be defined in the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

dcristoloveanu
dcristoloveanu reviewed Oct 25, 2024
View reviewed changes
win32/src/threadpool_win32.c
interlocked_add(&timer->is_destroying, 0) == 1
)
{
LogError("timer is in closing state");
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closing

I thought the states are CREATED/DESTROYING/DESTROYED.
A wild closing appears :-) #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified

dcristoloveanu
dcristoloveanu reviewed Oct 25, 2024
View reviewed changes
win32/src/threadpool_win32.c
@@ -625,6 +627,14 @@ int threadpool_timer_restart(TIMER_INSTANCE_HANDLE timer, uint32_t start_delay_m
timer, start_delay_ms, timer_period_ms);
result = MU_FAILURE;
}
else if(
/* Codes_SRS_THREADPOOL_WIN32_07_001: [ If timer state is in destroying state, threadpool_timer_restart shall fail and return a non - zero value. ]*/
interlocked_add(&timer->is_destroying, 0) == 1
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_destroying

Can we have this as a proper enum? With the 3 states we have #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

dcristoloveanu
dcristoloveanu reviewed Oct 25, 2024
View reviewed changes
win32/devdoc/threadpool_win32.md
@@ -245,15 +245,15 @@ MOCKABLE_FUNCTION(, void, threadpool_timer_destroy, TIMER_INSTANCE_HANDLE, timer

**SRS_THREADPOOL_WIN32_42_011: [** If `timer` is `NULL`, `threadpool_timer_destroy` shall fail and return. **]**

`threadpool_timer_destroy` shall switch the state to `DESTROYING`.
**SRS_THREADPOOL_WIN32_07_002: [** `threadpool_timer_destroy` shall indicate the timer is in destroying state. **]**
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[** `threadpool_t

I find these specs rather poor.
As a reader I can only try to imagine what we want to achieve.
I think we should describe the states we have and why we have them:

I assume we have something along the lines of:
CREATED - state in which the timer is after threadpool_timer_start was called.
DESTROYING - state in which the timer is after threadpool_timer_destroy was called. A threadpool_timer_restart call after switching to DESTROYING state is failed.
DESTROYED - terminal state for the timer. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

dcristoloveanu
dcristoloveanu requested changes Oct 25, 2024
View reviewed changes
Copy link
Member

@dcristoloveanu dcristoloveanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

@azure-pipelines Azure Pipelines
Copy link

No pipelines are associated with this pull request.

1 similar comment
@azure-pipelines Azure Pipelines
Copy link

No pipelines are associated with this pull request.

@dcristoloveanu
Copy link
Member

MOCKABLE_FUNCTION(, void, threadpool_timer_destroy, TIMER_INSTANCE_HANDLE, timer);

One more interesting comment on this.
If this handle woutl be THANDLE, isn't it true that we could not have by design a race where restart is called while destroy happens?

Refers to: win32/devdoc/threadpool_win32.md:241 in d68b9c8. [](commit_id = d68b9c8, deletion_comment = False)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasmineymlo jasmineymlo jasmineymlo left review comments

@dcristoloveanu dcristoloveanu dcristoloveanu requested changes

@anporumb anporumb Awaiting requested review from anporumb

@avranju avranju Awaiting requested review from avranju

@mattdurak mattdurak Awaiting requested review from mattdurak

@jebrando jebrando Awaiting requested review from jebrando

@darobs darobs Awaiting requested review from darobs

@ipg2013 ipg2013 Awaiting requested review from ipg2013

@parth21999 parth21999 Awaiting requested review from parth21999

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@M-iceberg @jasmineymlo @dcristoloveanu